security: fix command injection in Telegram bridge#119
security: fix command injection in Telegram bridge#119kakuteki wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
kakuteki
force-pushed
the
fix/telegram-bridge-command-injection
branch
from
10a0335 to
7b1dfd9
Compare
User messages from Telegram were interpolated directly into a shell command string with only single-quote escaping, allowing attackers to execute arbitrary commands via $() or backtick expansion. Fix: pass both the API key and user message through stdin instead of the command string, validate sessionId and SANDBOX_NAME, and change stdio from "ignore" to "pipe" so stdin is writable.
kakuteki
force-pushed
the
fix/telegram-bridge-command-injection
branch
from
7b1dfd9 to
fa74697
Compare
📝 WalkthroughWalkthroughThe pull request addresses a command injection vulnerability in the Telegram Bridge by sanitizing inputs (session ID and SANDBOX name), validating characters, and refactoring data passage from command-line embedding to stdin, preventing attackers from executing arbitrary commands via malicious messages. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/telegram-bridge.js (1)
25-27: Consider making ALLOWED_CHAT_IDS mandatory for production deployments.The optional
ALLOWED_CHAT_IDSwhitelist is a good defense-in-depth mechanism. For production, enforcing this (or at least emitting a warning when unset) would reduce attack surface by ensuring only trusted Telegram chats can interact with the agent.const ALLOWED_CHATS = process.env.ALLOWED_CHAT_IDS ? process.env.ALLOWED_CHAT_IDS.split(",").map((s) => s.trim()) : null; + +if (!ALLOWED_CHATS) { + console.warn("WARNING: ALLOWED_CHAT_IDS not set — all Telegram chats can interact with this bot"); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/telegram-bridge.js` around lines 25 - 27, Make ALLOWED_CHAT_IDS mandatory (or emit a clear warning) by checking process.env.ALLOWED_CHAT_IDS at startup and failing fast or logging a high‑severity warning if it's missing; update the ALLOWED_CHATS initialization (the variable ALLOWED_CHATS and the process.env.ALLOWED_CHAT_IDS usage) so that when the env var is absent the process either exits with a descriptive error message or logs a prominent warning advising this must be set in production, and ensure the message includes guidance on the expected comma-separated format.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/telegram-bridge.js`:
- Around line 25-27: Make ALLOWED_CHAT_IDS mandatory (or emit a clear warning)
by checking process.env.ALLOWED_CHAT_IDS at startup and failing fast or logging
a high‑severity warning if it's missing; update the ALLOWED_CHATS initialization
(the variable ALLOWED_CHATS and the process.env.ALLOWED_CHAT_IDS usage) so that
when the env var is absent the process either exits with a descriptive error
message or logs a prominent warning advising this must be set in production, and
ensure the message includes guidance on the expected comma-separated format.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9730f601-7e91-4bf6-908f-ec6ee23bb938
📒 Files selected for processing (1)
scripts/telegram-bridge.js
|
Hi maintainers, could you please approve the CI workflow run for this PR? I've rebased onto the latest main. Thank you! |
north-echo
left a comment
north-echo
left a comment
There was a problem hiding this comment.
Security Review, PR #119
The stdin-based approach for passing the API key and user message is the right call here. It eliminates shell interpretation of user input on the remote side entirely. This is the correct fix for the core command injection (issue #118). A few gaps worth addressing:
- execSync still used for ssh-config (line 95)
const sshConfig = execSync(openshell sandbox ssh-config ${SANDBOX}, { encoding: "utf-8" });
Even though SANDBOX is validated at startup, this is still string interpolation passed to a shell. PR #320 gets this right with execFileSync("openshell", ["sandbox", "ssh-config", SANDBOX]). Defense in depth: the validation is one layer, parameterized execution is another.
- SANDBOX regex allows leading hyphens
if (!/^[a-zA-Z0-9_-]+$/.test(SANDBOX)) {
This matches -v, --help, etc., which could be interpreted as options by openshell. PR #320's pattern is better:
/^[A-Za-z0-9][A-Za-z0-9_-]*$/
Requiring an alphanumeric first character prevents option injection.
- Predictable temp file path
const confPath = /tmp/nemoclaw-tg-ssh-${safeSessionId}.conf;
safeSessionId is derived from the Telegram chat ID, a public integer. On multi-user systems this is a symlink race target (CWE-377). PR #320 solves this with crypto.randomBytes(8).toString("hex") and exclusive creation (flag: "wx").
- No message length cap
Unbounded user input passed to the agent. A reasonable cap (e.g., 4096 chars matching Telegram's own limit) would prevent abuse.
- Bare openshell vs resolved path
The script imports resolveOpenshell() and validates it at startup, but the execSync call on line 95 uses bare openshell instead of the resolved OPENSHELL path.
Recommendation
This PR and #320 are complementary, not competing. This PR fixes the core injection; #320 fixes the surrounding hardening (execFileSync, temp file races, better SANDBOX regex). Combining both would give a comprehensive fix. Consider adopting #320's hardening into this branch, or coordinating with @pjt222.
The stdin approach itself is sound.
Christopher Lusk (christopherdlusk@gmail.com)
|
Thanks for identifying and fixing the command injection vulnerability, this is a critical security fix that will help protect NemoClaw users. |
wscurran
added
the
priority: high
|
@north-echo Thank you for the thorough security review. I'll coordinate with @pjt222 to address the points you raised. |
wscurran
added
the
Integration: Telegram
|
Not a duplicate of #617 (bridge framework). Your stdin-based approach (passing the API key and message via stdin instead of shell interpolation) is a different and arguably more robust mitigation than #617's However, #617 rewrites Worth coordinating with #617 on merge order. |
…nitization Adds two new Brev E2E test suites targeting the vulnerabilities fixed by PR NVIDIA#119 (Telegram bridge command injection) and PR NVIDIA#156 (credential exposure in migration snapshots + blueprint digest bypass). Test suites: - test-telegram-injection.sh: 8 tests covering command substitution, backtick injection, quote-breakout, parameter expansion, process table leaks, and SANDBOX_NAME validation - test-credential-sanitization.sh: 13 tests covering auth-profiles.json deletion, credential field stripping, non-credential preservation, symlink safety, blueprint digest verification, and pattern-based field detection These tests are expected to FAIL on main (unfixed code) and PASS once PR NVIDIA#119 and NVIDIA#156 are merged. Refs: NVIDIA#118, NVIDIA#119, NVIDIA#156, NVIDIA#813
Adds specification for a warm-pool model that eliminates the 40+ min cold-start Docker build time from Brev E2E test runs. Key design: - Pre-warmed Brev instances using NemoClaw launchable - Naming convention (e2e-warm-*) as state — no external tracking - PR comment trigger (/test-brev <suites>) for maintainers - Parallel test execution across pool instances - Daily health check + 24h instance cycling - Fallback to ephemeral mode if pool is empty 6 phases: pool script → warmer workflow → test runner → PR trigger → run security tests → cleanup Also includes the original NVIDIA#813 hardware E2E spec for reference. Refs: NVIDIA#813, NVIDIA#118, NVIDIA#119, NVIDIA#156
…nitization Adds two new Brev E2E test suites targeting the vulnerabilities fixed by PR NVIDIA#119 (Telegram bridge command injection) and PR NVIDIA#156 (credential exposure in migration snapshots + blueprint digest bypass). Test suites: - test-telegram-injection.sh: 8 tests covering command substitution, backtick injection, quote-breakout, parameter expansion, process table leaks, and SANDBOX_NAME validation - test-credential-sanitization.sh: 13 tests covering auth-profiles.json deletion, credential field stripping, non-credential preservation, symlink safety, blueprint digest verification, and pattern-based field detection These tests are expected to FAIL on main (unfixed code) and PASS once PR NVIDIA#119 and NVIDIA#156 are merged. Refs: NVIDIA#118, NVIDIA#119, NVIDIA#156, NVIDIA#813
…al sanitization (#1092) * test(security): add E2E tests for command injection and credential sanitization Adds two new Brev E2E test suites targeting the vulnerabilities fixed by PR #119 (Telegram bridge command injection) and PR #156 (credential exposure in migration snapshots + blueprint digest bypass). Test suites: - test-telegram-injection.sh: 8 tests covering command substitution, backtick injection, quote-breakout, parameter expansion, process table leaks, and SANDBOX_NAME validation - test-credential-sanitization.sh: 13 tests covering auth-profiles.json deletion, credential field stripping, non-credential preservation, symlink safety, blueprint digest verification, and pattern-based field detection These tests are expected to FAIL on main (unfixed code) and PASS once PR #119 and #156 are merged. Refs: #118, #119, #156, #813 * ci: temporarily disable repo guard for fork testing * ci: bump bootstrap timeout, skip vLLM on CPU E2E runs - Add SKIP_VLLM=1 support to brev-setup.sh - Use SKIP_VLLM=1 in brev-e2e.test.js bootstrap - Bump beforeAll timeout to 30 min for CPU instances - Bump workflow timeout to 60 min for 3 test suites * ci: bump bootstrap timeout to 40 min for sandbox image build * ci: bump Brev instance to 8x32 for faster Docker builds * ci: add real-time progress streaming for E2E bootstrap and tests - Stream SSH output to CI log during bootstrap (no more silence) - Add timestamps to brev-setup.sh and setup.sh info/warn/fail messages - Add background progress reporter during sandbox Docker build (heartbeat every 30s showing elapsed time, current Docker step, and last log line) - Stream test script output to CI log via tee + capture for assertions - Filter potential secrets from progress heartbeat output * ci: use NemoClaw launchable for E2E bootstrap Replace bare 'brev create' + brev-setup.sh with 'brev start' using the OpenShell-Community launch-nemoclaw.sh setup script. This installs Docker, OpenShell CLI, and Node.js via the launchable's proven path, then runs 'nemoclaw onboard --non-interactive' to build the sandbox (testing whether this path is faster than our manual setup.sh). Changes: - Default CPU back to 4x16 (8x32 didn't help — bottleneck was I/O) - Launchable path: brev start + setup-script URL, poll for completion, rsync PR branch, npm ci, nemoclaw onboard - Legacy path preserved (USE_LAUNCHABLE=0) - Timestamped logging throughout for timing comparison - New use_launchable workflow input (default: true) * fix: prevent openshell sandbox create from hanging in non-interactive mode openshell sandbox create without a command defaults to opening an interactive shell inside the sandbox. In CI (non-interactive SSH), this hangs forever — the sandbox goes Ready but the command never returns. The [?2004h] terminal escape codes in CI logs were bash enabling bracketed paste mode, waiting for input. Add --no-tty -- true so the command exits immediately after the sandbox is created and Ready. * fix: source nvm in non-interactive SSH for launchable path The launchable setup script installs Node.js via nvm, which sets up PATH in ~/.nvm/nvm.sh. Non-interactive SSH doesn't source .bashrc, so npm/node commands fail with 'command not found'. Source nvm.sh before running npm in the launchable path and runRemoteTest. * fix: setup.sh respects NEMOCLAW_SANDBOX_NAME env var setup.sh defaulted to 'nemoclaw' ignoring the NEMOCLAW_SANDBOX_NAME env var set by the CI test harness (e2e-test). Now uses $1 > $NEMOCLAW_SANDBOX_NAME > nemoclaw. * ci: bump full E2E test timeout to 15 min for install + sandbox build * ci: don't run full E2E alongside security tests (it destroys the sandbox) The full E2E test runs install.sh --non-interactive which destroys and rebuilds the sandbox. When TEST_SUITE=all, this kills the sandbox that beforeAll created, causing credential-sanitization and telegram-injection to fail with 'sandbox not running'. Only run full E2E when TEST_SUITE=full. * ci: pre-build base image locally when GHCR image unavailable On forks or before the first base-image workflow run, the GHCR base image (ghcr.io/nvidia/nemoclaw/sandbox-base:latest) doesn't exist. This causes the Dockerfile's FROM to fail. Now setup.sh checks for the base image and builds Dockerfile.base locally if needed. On subsequent builds, Docker layer cache makes this near-instant. Once the GHCR base image is available, this becomes a no-op (docker pull succeeds and the local build is skipped). * ci: install nemoclaw CLI after bootstrap in non-launchable path brev-setup.sh creates the sandbox but doesn't install the host-side nemoclaw CLI that test scripts need for 'nemoclaw <name> status'. Add npm install + build + link step after bootstrap. * fix: use npm_config_prefix for nemoclaw CLI install so it lands on PATH * fix: npm link from repo root where bin.nemoclaw is defined * fix(ci): register sandbox in nemoclaw registry after setup.sh bootstrap setup.sh creates the sandbox via openshell directly but never writes ~/.nemoclaw/sandboxes.json. The security test scripts check `nemoclaw <name> status` which reads the registry, causing all E2E runs to fail with 'Sandbox e2e-test not running'. Write the registry entry after nemoclaw CLI install so the test scripts can find the sandbox. * style: shfmt formatting fix in setup.sh * fix(test): exclude policy presets from C7 secret pattern scan C7 greps for 'npm_' inside the sandbox and false-positives on nemoclaw-blueprint/policies/presets/npm.yaml which contains rule names like 'npm_yarn', not actual credentials. Filter out /policies/ paths from all three pattern checks. * docs(ci): add test suite descriptions to e2e-brev workflow header Document what each test_suite option runs so maintainers can make an informed choice from the Actions UI without reading the test scripts. * ci: re-enable repo guard for e2e-brev workflow Re-enable the github.repository check so the workflow only runs on NVIDIA/NemoClaw, not on forks. * fix(test): update setup-sandbox-name test for NEMOCLAW_SANDBOX_NAME env var setup.sh now uses ${1:-${NEMOCLAW_SANDBOX_NAME:-nemoclaw}} instead of ${1:-nemoclaw}. Update the test to match and add coverage for the env var fallback path. * fix(lint): add shellcheck directives for injection test payloads and fix stdio type * fix(lint): suppress SC2034 for status_output in credential sanitization test * fix: address CodeRabbit review — timeout, pipefail, fail-closed probes, shell injection in test - Bump e2e-brev workflow timeout-minutes from 60 to 90 - Add fail-fast when launchable setup exceeds 40-min wait - Add pipefail to remote pipeline commands in runRemoteTest and npm ci - Fix backtick shell injection in validateName test loop (use process.argv) - Make sandbox_exec fail closed with __PROBE_FAILED__ sentinel - Add probe failure checks in C6/C7 sandbox assertions --------- Co-authored-by: Carlos Villela <cvillela@nvidia.com>
|
Hi! Thanks for this contribution. It looks like this branch has fallen a bit behind main and has some merge conflicts. Could you please rebase or merge with main to resolve the conflicts? Let me know if you need any help. Thanks! |
6 participants
Summary
Fixes #118 — Command injection vulnerability in
scripts/telegram-bridge.jsThe Telegram bridge interpolated user messages directly into a shell command string passed to SSH. The existing escaping (single-quote only) did not prevent
$()or backtick expansion, allowing attackers to execute arbitrary commands inside the sandbox and potentially exfiltrate theNVIDIA_API_KEY.Changes
read -r(line 1) and the message withcat(remaining stdin), then expands them inside double-quoted"$VAR"which is not subject to further shell parsingsessionId— strip non-alphanumeric characters (Telegram chat IDs are numeric)SANDBOX_NAMEat startup — reject names with shell metacharacters to prevent injection in theexecSynccallstdiofrom["ignore", ...]to["pipe", ...]so stdin is writableBefore (vulnerable)
After (fixed)
Test plan
$(whoami)→ treated as literal text, no command executionSANDBOX_NAME(e.g.foo;rm -rf /) → startup exits with errorNVIDIA_API_KEYno longer appears in process arguments (ps aux)Summary by CodeRabbit